Repository navigation
feat: bind exposition writers to negotiated escaping - #2642
zeitlinger wants to merge 6 commits into
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
|
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
jaydeluca
left a comment
jaydeluca
left a comment
There was a problem hiding this comment.
Thanks — this matches the shape sketched in #1577, and moving the OM2 fallback into
ExpositionFormats means manual callers get it too, not just the scrape handler.
- Escaping is still parsed from the whole Accept header rather than the selected media type
(inline below). Pre-existing, but this method now promises both come from one negotiation. - A couple of small javadoc/test wording nits.
| fallback = EscapingScheme.ALLOW_UTF8; | ||
| } | ||
| return new ExpositionFormatWriterWithEscaping( | ||
| writer, EscapingScheme.fromAcceptHeader(acceptHeader, fallback)); |
There was a problem hiding this comment.
EscapingScheme.fromAcceptHeader splits only on ;, so when escaping= is the last parameter of
a media type it absorbs the next one and is ignored:
application/openmetrics-text;version=1.0.0;escaping=allow-utf-8,text/plain;version=0.0.4;q=0.5
-> OpenMetrics 1.0 writer, UNDERSCORE_ESCAPING
It also lets an escaping= on a media type that wasn't selected apply to the one that was.
Prometheus's own header works because ;q= follows escaping=, and main behaves the same, so
this isn't a regression. But since this method is now the single place where format and escaping
are negotiated, could it read escaping from the media type that findWriter matched (split on
, first)?
| * ExpositionFormats.init().findWriterWithEscaping(acceptHeader).write(out, registry.scrape()); | ||
| * }</pre> | ||
| * | ||
| * @see #findWriter(String) the legacy API for selecting only a format writer |
There was a problem hiding this comment.
findWriter isn't deprecated and the docs say it "remains available", so "legacy" sends a mixed
signal.
| * @see #findWriter(String) the legacy API for selecting only a format writer | |
| * @see #findWriter(String) to select only the format writer |
| // Negotiation binds a scheme to this wrapper without changing the cached format writer. | ||
| assertThat(formats.findWriterWithEscaping("text/plain").getEscapingScheme()) | ||
| .isEqualTo(EscapingScheme.DEFAULT); |
There was a problem hiding this comment.
The assertion checks a new wrapper's scheme, not that the cached writer is unchanged. To test
what the comment says:
| // Negotiation binds a scheme to this wrapper without changing the cached format writer. | |
| assertThat(formats.findWriterWithEscaping("text/plain").getEscapingScheme()) | |
| .isEqualTo(EscapingScheme.DEFAULT); | |
| // Negotiation wraps the shared format writer rather than replacing it. | |
| assertThat(formats.findWriter("text/plain; escaping=" + escaping)) | |
| .isSameAs(formats.getPrometheusTextFormatWriter()); |
| * format's standard content type and does not include an escaping parameter. | ||
| */ | ||
| @StableApi | ||
| public final class ExpositionFormatWriterWithEscaping { |
There was a problem hiding this comment.
Since this is @StableApi: was leaving out an accessor for the underlying ExpositionFormatWriter
deliberate? Without one, callers can't check isAvailable() or which format was picked other
than by comparing content types. Adding it later is non-breaking, so fine either way.
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
I added an immutable
ExpositionFormatWriterWithEscapingwrapper and a factory that binds the selected format to the escaping scheme negotiated from theAcceptheader. The scrape handler now uses the negotiated writer, while the existing writer API and standard content types remain unchanged. Fixes #1577.I added the generated API diff report and normalized its generator to omit trailing blank lines rejected by lint. Lint and unit tests pass. The local
mise run buildreached integration tests but Docker/Testcontainers failures prevented the full build; the install build with integration tests skipped passed.I also documented manual negotiated responses in the Formats guide.